-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding name col to CrontabSchedule model #478
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also pull from master please
@@ -0,0 +1,18 @@ | |||
# Generated by Django 3.1.7 on 2021-12-15 13:06 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use django 3.2.x to regenerate this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-created with 3.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trianglesis It doesn't seem necessary to do the if
twice. I'll checkout your branch in my projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
…y Django 3.2 (venv) PS D:\Projects\..\django-celery-beat> python -m django --version 3.2
@lvelvee kindly have a review when have time |
django_celery_beat/models.py
Outdated
|
||
name = None | ||
if self.name: | ||
name = '[{}]'.format(self.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I wanted to achieve [Name in brackets], but leave an empty '' if there is no name.
Making if and if - makes it more visible on code, but It could be changed back to 1st version.
Thanks!
@@ -299,6 +299,13 @@ class CrontabSchedule(models.Model): | |||
'Timezone to Run the Cron Schedule on. Default is UTC.'), | |||
) | |||
|
|||
name = models.CharField( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would also like to have unit tests for new proposed changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't run any test from test_schedulers.py with the current setup.
Will try to figure out what am I doing wrong, and possibly add some tests.
~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by. ~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab". ~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block. models.py ~ CrontabSchedule: simpler name for crontab ~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name.
@@ -116,9 +116,9 @@ class PeriodicTaskAdmin(admin.ModelAdmin): | |||
model = PeriodicTask | |||
celery_app = current_app | |||
date_hierarchy = 'start_time' | |||
list_display = ('__str__', 'enabled', 'interval', 'start_time', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why str is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using str there is no possible way to sort\group tasks by name.
Name is an essential feature that can be used to make sort easy and cron tasks more meaningful.
In this example, I can add prefixes in the name, and be able to group all tasks by any logical human-understandable meaning.
In addition, I think there is no need to add extra fields in the name when we have separate Crontab, Interval columns in the table already.
~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by. ~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab". ~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block. models.py ~ CrontabSchedule: simpler name for crontab ~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name. admin.py ~ Added view form for CrontabSchedule, sorting by name, display name, __str__, timezone, made easy to sort\group items, rename, fix or delete obsolete ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit tests for the changes to avoid possible regression?
I'll try to add tests as soon as I figure out how to execute them all. Unfortunately, I can't get to work few tests related to model testing, which I wanted to use as a draft\example to test my changes. |
~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by. ~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab". ~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block. models.py ~ CrontabSchedule: simpler name for crontab ~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name. admin.py ~ Added view form for CrontabSchedule, sorting by name, display name, __str__, timezone, made easy to sort\group items, rename, fix or delete obsolete ones. unit test_models.py + Add simple test on creation named crontab entry with possible valid names.
~ list_display: added name of the "Periodic Task" rather than str: it made the name field easy to read and sort by. ~ list_filter: added "crontab" sequence to the very end, it helps to sort all "Periodic Task"s related to selected "crontab". ~ Arguments: not collapsed, usually when you're editing a task - you edit crontab or arguments. No need to click each time to open collapsed block. models.py ~ CrontabSchedule: simpler name for crontab ~ PeriodicTask: do not show interval or crontab in the name of the task, since we have a table col for that already. It also enabled sort by this name. admin.py ~ Added view form for CrontabSchedule, sorting by name, display name, __str__, timezone, made easy to sort\group items, rename, fix or delete obsolete ones. django_celery_beat models.py ~ There was an extra space on __str__ when name is not set. Now it shouldn't appear. unit test_models.py + Add simple test on creation named crontab entry with possible valid names. unit test_schedulers.py + Assert PeriodicTask name is in correct format and have a space and [] braces. ~ Assert PeriodicTask name without cron\interval appendix. + Assert PeriodicTask cron\interval __str__ is expected in human-readable format.
Currently
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please rebase the conflicts please?
Having a lot of Cron time schedule items is frustrating when they have no verbose and short names, for example.
I'm trying to add a name to each - so it will be more visible, what task run on which time, actually.
With this change, it becomes more visible for the human eye what schedule is used for each task.
It's also better visible when you want to switch schedules, but the dropdown list is too big.
In addition: for years of usage - I have collected dozens of unused schedules and it starting to become a nightmare when you want to clean it up.